feat(pinned events): persist the pinned events timeline#6085
Conversation
d6a6eed to
046adb4
Compare
fb23758 to
7f1f465
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6085 +/- ##
==========================================
- Coverage 89.88% 89.81% -0.08%
==========================================
Files 363 363
Lines 100129 100479 +350
Branches 100129 100479 +350
==========================================
+ Hits 90004 90246 +242
- Misses 6621 6695 +74
- Partials 3504 3538 +34 ☔ View full report in Codecov by Sentry. |
poljar
left a comment
There was a problem hiding this comment.
Alright, I left some comments. Most of them are minor so I'll approve ahead of time.
| // Only handle the remote aggregation for a non-live timeline, that's not the | ||
| // pinned events one (since this one handles its own remote events diffs). |
There was a problem hiding this comment.
I'm not sure that this is needed for pinned events.
There was a problem hiding this comment.
Hmm, I thought the live update task wouldn't be needed for the pinned events timeline, but that's actually unclear; it might be that the member profiles updates are still useful for such a timeline. Will defer this to a subsequent PR, and fix the comment in the meanwhile.
crates/matrix-sdk-ui/tests/integration/timeline/pinned_event.rs
Outdated
Show resolved
Hide resolved
Now that this logic has moved to the event cache, it's not required in the timeline anymore.
… configure the event cache
…rovider trait And get rid of the `PinnedEventsRoom` trait, and the accompanying file.
…ions` now that it's widely available
|
Clearing the other review requests, since Damir has reviewed this. Thanks! |
7f1f465 to
595e6cc
Compare
|
We might want to delay merging this, I found an issue while testing it with EXA where after clearing the cache, some rooms with just 1 pinned event incorrectly displays several of them. |
…cache There were two issues: - first, `load_or_fetch_event_with_relations()` allowed to pass a filter, but the filter wasn't taken into account when fetching relations from the network. This would cause the initial load of pinned events to also include thread responses, which we don't want. - similarly, when adding related events from sync, we'd only look if an event had a `m.relates_to` field; but it could be a thread response being added in live. The two issues are fixed similarly, by using a new `extract_relation` serde helper that gives both the related_to event and the relation type. That way, we can apply a manual filter in `load_or_fetch_event_with_relations` after fetching relations from network, and we can filter out live events based on the relation type.
|
The latest changes fix the previously found issue 🥳 ! |
This brings some nice benefits:
Some important design points:
Part of #5954.